Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(terraform_docs): Fix non-GNU sed issues, introduced in v1.93.0 #704

Merged
merged 2 commits into from
Aug 29, 2024
Merged

fix(terraform_docs): Fix non-GNU sed issues, introduced in v1.93.0 #704

merged 2 commits into from
Aug 29, 2024

Conversation

cschroer
Copy link
Contributor

Ensure compatibility of terraform_docs hook on MacOS and Linux by using GNU sed.

  • This PR introduces breaking change.
  • This PR fixes a bug.
  • This PR adds new functionality.
  • This PR enhances existing functionality.

Description of your changes

In PR #701, the usage of sed was introduced to the terraform_docs for in-file changes, resulting in #703

MacOS requires an extension for the -i parameter, unlike GNU sed.
As a result the hook fails on MacOS know with the following error:

Terraform docs..........................................................................Failed
- hook id: terraform_docs
- exit code: 1

sed: 1: "README.md": invalid command code R

I see two potential solutions for this issue.

  1. Always pass a extension to the -i parameter, as this is compatible with GNU sed, too. As a result, a backup file would be created for each sed commands operation and needs to be deleted again.

  2. Ensure we always use GNU sed. GNU sed is available via brew and can easily be installed on MacOS systems. Using GNU sed on both MacOS and Linux ensures consistent behaviour across operating systems.

I'm happy to change it to option 1, but wanted to show both options I see to fix the issue. Also I'm not 100% sure about Windows support for this hooks, but it seems like ( #648 ) they only work on Linux and MacOS.

@drAlberT
Copy link

drAlberT commented Aug 29, 2024 •
edited
Loading

@cschroer @MaxymVlasov @yermulnik there is a much more easy solution, variant of option 1 (I'm using it for many years and it is proven to be portable) ...

When you use "-i" in sed you must always add the backup suffix otherwise it would not work on Mac for example. Adding and empty suffix does the trick:

# bad
infra albert$ sed -i "s/^ASDDFGFDSGHSDFG$/PIPPOPIPPO/" "MODULE-DETAILS.md" 
sed: 1: "MODULE-DETAILS.md": invalid command code M

#good
infra albert$ sed -i "" "s/^ASDDFGFDSGHSDFG$/PIPPOPIPPO/" "MODULE-DETAILS.md"
infra albert$

This way no backup file is created, of course ;)

@yermulnik yermulnik linked an issue Aug 29, 2024 that may be closed by this pull request
terraform_docsfails with invalid command code R #703
Closed
yermulnik
yermulnik requested changes Aug 29, 2024
View reviewed changes
hooks/terraform_docs.sh Outdated Show resolved Hide resolved
@yermulnik yermulnik mentioned this pull request Aug 29, 2024
Closed
@MaxymVlasov MaxymVlasov changed the title fix(terraform_docs): Always use GNU sed fix(terraform_docs): Fix non-GNU sed issues, introduced in v1.93.0 Aug 29, 2024
@MaxymVlasov
Copy link
Collaborator

MaxymVlasov commented Aug 29, 2024 •
edited
Loading

And I suppose we need same fix in

pre-commit-terraform/README.md

Lines 585 to 590 in ad5d40b

To migrate everything to `terraform-docs` insertion markers, run in repo root:
```bash
grep -rl 'BEGINNING OF PRE-COMMIT-TERRAFORM DOCS HOOK' . | xargs sed -i 's/BEGINNING OF PRE-COMMIT-TERRAFORM DOCS HOOK/BEGIN_TF_DOCS/g'
grep -rl 'END OF PRE-COMMIT-TERRAFORM DOCS HOOK' . | xargs sed -i 's/END OF PRE-COMMIT-TERRAFORM DOCS HOOK/END_TF_DOCS/g'
```

@cschroer
fix(terraform_docs): Always use GNU sed
de569e0 
Ensure sed commands are compatible with MacOS and Linux/GNO version
@cschroer
Copy link
Contributor Author

I personally ran into some other issues with sed on MacOS, therefor I personally prefer using GNU sed on MacOS, too.
But for this case it's maybe better to simply use the empty extension. 👍 Adjusted the code and also updated the README.

yermulnik
yermulnik reviewed Aug 29, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
hooks/terraform_docs.sh Outdated Show resolved Hide resolved
@MaxymVlasov @yermulnik
Apply suggestions from code review
c5c6dcb 
Co-authored-by: George L. Yermulnik <[email protected]>
@MaxymVlasov MaxymVlasov requested a review from yermulnik August 29, 2024 16:57
@MaxymVlasov MaxymVlasov added bug Something isn't working hook/terraform_docs Bash hook labels Aug 29, 2024
@MaxymVlasov
Copy link
Collaborator

Also I'm not 100% sure about Windows support for this hooks, but it seems like ( #648 ) they only work on Linux and MacOS.

@cschroer yep, you're right
From installation instructions,

pre-commit-terraform/README.md

Lines 207 to 213 in 99fceb8

<details><summary><b>Windows 10/11</b></summary>
We highly recommend using [WSL/WSL2](https://docs.microsoft.com/en-us/windows/wsl/install) with Ubuntu and following the Ubuntu installation guide. Or use Docker.
> [!IMPORTANT]
> We won't be able to help with issues that can't be reproduced in Linux/Mac.
> So, try to find a working solution and send PR before open an issue.

@MaxymVlasov MaxymVlasov merged commit 3c8734d into antonbabenko:master Aug 29, 2024
5 checks passed
antonbabenko pushed a commit that referenced this pull request Aug 29, 2024
@semantic-release-bot
chore(release): version 1.93.1 [skip ci]
12a54be 
## [1.93.1](v1.93.0...v1.93.1) (2024-08-29)

### Bug Fixes

* **`terraform_docs`:** Fix non-GNU `sed` issues, introduced in v1.93.0 ([#704](#704)) ([3c8734d](3c8734d))
@antonbabenko
Copy link
Owner

This PR is included in version 1.93.1 🎉

@BeneHa BeneHa mentioned this pull request Aug 30, 2024
Closed
@cschroer cschroer deleted the fix/gnu-sed branch August 30, 2024 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drAlberT drAlberT drAlberT left review comments

@robinbowes robinbowes robinbowes left review comments

@MaxymVlasov MaxymVlasov MaxymVlasov approved these changes

@yermulnik yermulnik yermulnik approved these changes

Assignees
No one assigned
Labels
bug Something isn't working hook/terraform_docs Bash hook
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

terraform_docsfails with invalid command code R
6 participants
@cschroer @drAlberT @MaxymVlasov @antonbabenko @robinbowes @yermulnik